-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Output SUTRA pump efficiency as percentage (issue 95) #275
base: main
Are you sure you want to change the base?
Conversation
src/geophires_x/SUTRAOutputs.py
Outdated
@@ -156,7 +156,7 @@ def PrintOutputs(self, model: Model): | |||
f.write(f" Well Depth: {model.reserv.depth.value:10.1f} " + model.reserv.depth.CurrentUnits.value + NL) | |||
|
|||
pump_efficiency_display_unit = model.surfaceplant.pump_efficiency.CurrentUnits.value | |||
pump_efficiency_display = f'{model.surfaceplant.pump_efficiency.value:10.1f} {pump_efficiency_display_unit}' | |||
pump_efficiency_display = f'{model.surfaceplant.pump_efficiency.value * 100:10.1f} {pump_efficiency_display_unit}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory we shouldn't have to multiply by 100 here because conversion from current units to preferred units should be happening beforehand and doing it for us. Do you know why that apparently isn't the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...ah, I see that the relevant code is commented out, and always has been:
GEOPHIRES-X/src/geophires_x/SUTRAOutputs.py
Lines 87 to 107 in 0568ba7
# Deal with converting Units back to PreferredUnits, if required. | |
# before we write the outputs, we go thru all the parameters for all of the objects and set the values back | |
# to the units that the user entered the data in | |
# We do this because the value may be displayed in the output, and we want the user to recognize their value, | |
# not some converted value | |
# for obj in [model.reserv, model.wellbores, model.surfaceplant, model.economics]: | |
# for key in obj.ParameterDict: | |
# param = obj.ParameterDict[key] | |
# if not param.UnitsMatch: ConvertUnitsBack(param, model) | |
# now we need to loop through all thw output parameters to update their units to | |
# whatever units the user has specified. | |
# i.e., they may have specified that all LENGTH results must be in feet, so we need to convert those | |
# from whatever LENGTH unit they are to feet. | |
# same for all the other classes of units (TEMPERATURE, DENSITY, etc). | |
#for obj in [model.reserv, model.wellbores, model.surfaceplant, model.economics]: | |
# for key in obj.OutputParameterDict: | |
# if key in self.ParameterDict: | |
# if self.ParameterDict[key] != obj.OutputParameterDict[key].CurrentUnits: | |
# ConvertOutputUnits(obj.OutputParameterDict[key], self.ParameterDict[key], model) |
I presume this commenting is an unintentional oversight from copy-pasting SUTRAOutputs.py
from Outputs.py
. Let's replace the commented code in question with the current equivalent in
GEOPHIRES-X/src/geophires_x/Outputs.py
Lines 740 to 764 in b7a6d86
# Deal with converting Units back to PreferredUnits, if required. | |
# before we write the outputs, we go thru all the parameters for all of the objects and set the values back | |
# to the units that the user entered the data in | |
# We do this because the value may be displayed in the output, and we want the user to recginze their value, | |
# not some converted value | |
for obj in [model.reserv, model.wellbores, model.surfaceplant, model.economics]: | |
for key in obj.ParameterDict: | |
param = obj.ParameterDict[key] | |
if not param.UnitsMatch: | |
ConvertUnitsBack(param, model) | |
# now we need to loop through all the output parameters to update their units to | |
# whatever units the user has specified. | |
# i.e., they may have specified that all LENGTH results must be in feet, so we need to convert those | |
# from whatever LENGTH unit they are to feet. | |
# same for all the other classes of units (TEMPERATURE, DENSITY, etc). | |
for obj in [model.reserv, model.wellbores, model.surfaceplant, model.economics]: | |
for key in obj.OutputParameterDict: | |
output_param:OutputParameter = obj.OutputParameterDict[key] | |
if key in self.ParameterDict: | |
if self.ParameterDict[key] != output_param.CurrentUnits: | |
ConvertOutputUnits(output_param, self.ParameterDict[key], model) | |
elif not output_param.UnitsMatch: | |
obj.OutputParameterDict[key] = output_param.with_preferred_units() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this, but it does not produce the desired output. Without the explicit multiplication by 100 (as in line 159 above), pump efficiency is output as 0.8 %
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like it'll require debugger analysis
Per sync discussion: this is the problematic line GEOPHIRES-X/src/geophires_x/Parameter.py Line 604 in 95e2122
|
No description provided.